Skip to content

[SPARK-51473][ML][CONNECT] ML transformed dataframe keep a reference to the model - #50199

Closed
zhengruifeng wants to merge 7 commits into
apache:masterfrom
zhengruifeng:ml_connect_model_ref
Closed

[SPARK-51473][ML][CONNECT] ML transformed dataframe keep a reference to the model#50199
zhengruifeng wants to merge 7 commits into
apache:masterfrom
zhengruifeng:ml_connect_model_ref

Conversation

@zhengruifeng

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

add the model link in the transformed dataframe

Why are the changes needed?

#49948 disabled the model GC for fit_transform, this PR add the model link in the transformed dataframe, so that the model will be GCed together with the transformed dataframe

Does this PR introduce any user-facing change?

no

How was this patch tested?

existing test should cover this change

Was this patch authored or co-authored using generative AI tooling?

no

@zhengruifeng
zhengruifeng force-pushed the ml_connect_model_ref branch 2 times, most recently from dd3b334 to 2a607be Compare March 11, 2025 17:32
@zhengruifeng zhengruifeng changed the title [WIP][ML][CONNECT] ML transformed dataframe keep a reference to the model [SPARK-51473][ML][CONNECT] ML transformed dataframe keep a reference to the model Mar 11, 2025
@zhengruifeng
zhengruifeng marked this pull request as ready for review March 11, 2025 18:33
@zhengruifeng
zhengruifeng force-pushed the ml_connect_model_ref branch from 2b78d14 to 7058de2 Compare March 11, 2025 20:00
Comment thread python/pyspark/ml/util.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the previous ConnectDataFrame for ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ConnectDataFrame is to build the connect DF, it is still used at the end of this method

Comment thread python/pyspark/ml/util.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For model summary, it might also return Dataframe, shall we make similar change ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should

@zhengruifeng
zhengruifeng force-pushed the ml_connect_model_ref branch from 7058de2 to af525d4 Compare March 12, 2025 21:46
Comment thread python/pyspark/ml/util.py Outdated

@zhengruifeng zhengruifeng Mar 12, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

case 1: summary.predictions
the DF predictions hold a reference to summary (which link to the model)

case 2: GMMModel.gaussiansDF
the DF gaussiansDF hold a reference to GMMModel

Comment thread python/pyspark/ml/classification.py Outdated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

training summary hold a reference to the model

Comment thread python/pyspark/ml/classification.py Outdated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the testing summary model.evaluate(df) hold a reference to the model

@zhengruifeng
zhengruifeng force-pushed the ml_connect_model_ref branch from af525d4 to 05f37bc Compare March 13, 2025 23:23
@zhengruifeng

Copy link
Copy Markdown
Contributor Author

ping @WeichenXu123

@WeichenXu123 WeichenXu123 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zhengruifeng
zhengruifeng deleted the ml_connect_model_ref branch March 20, 2025 00:58
@zhengruifeng

Copy link
Copy Markdown
Contributor Author

merged to master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants